-
Notifications
You must be signed in to change notification settings - Fork 357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add StoreKit2 button #7400
Add StoreKit2 button #7400
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils)
ios/MullvadVPN/View controllers/Account/AccountViewController.swift
line 305 at r1 (raw file):
} guard case let .received(oldProduct) = productState, let accountData = interactor.deviceState.accountData
is the let accountData
meant to be duplicated?
ios/MullvadVPN/Classes/AccessbilityIdentifier.swift
line 41 at r1 (raw file):
case logoutButton case purchaseButton case storekit2Button
nit: capitalisation; i.e., perhaps this should be storeKit2Button
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 7 files reviewed, 2 unresolved discussions (waiting on @acb-mv)
ios/MullvadVPN/Classes/AccessbilityIdentifier.swift
line 41 at r1 (raw file):
Previously, acb-mv wrote…
nit: capitalisation; i.e., perhaps this should be
storeKit2Button
?
You're correct!
ios/MullvadVPN/View controllers/Account/AccountViewController.swift
line 305 at r1 (raw file):
Previously, acb-mv wrote…
is the
let accountData
meant to be duplicated?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
ed515a1
to
911692b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get a 400 from the API, "INVALID_INPUT". Is that expected before server side is done?
Reviewed 4 of 7 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pinkisemils)
ios/MullvadVPN/Classes/AccessbilityIdentifier.swift
line 41 at r3 (raw file):
case logoutButton case purchaseButton case storeKit2Button
Nit: Unless we add E2E tests we don't need this. It doesn't hurt, but if this feature is being removed/renamed/other it seems likely we might forget to clean up this a11y id as well.
ios/MullvadVPN/View controllers/Account/AccountViewController.swift
line 347 at r3 (raw file):
private func sendReceiptToAPI(accountNumber: String, receipt: VerificationResult<Transaction>) async { do { // Accessing unsafe payload data because n
because "n"?
ios/MullvadVPN/View controllers/Account/PaymentAlertPresenter.swift
line 39 at r3 (raw file):
} func showAlertForStoreKitError(
We can use showAlertForError()
above instead.
911692b
to
d2e1fee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pinkisemils)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is to be expected. We might even change the button to use a different API call.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @acb-mv and @rablador)
ios/MullvadVPN/View controllers/Account/AccountViewController.swift
line 347 at r3 (raw file):
Previously, rablador (Jon Petersson) wrote…
because "n"?
We actually don't we even need it. It's a leftover from when I was dumping the payload for inspecting it in the debugger.
ios/MullvadVPN/View controllers/Account/PaymentAlertPresenter.swift
line 39 at r3 (raw file):
Previously, rablador (Jon Petersson) wrote…
We can use
showAlertForError()
above instead.
I don't think so, because it takes StorePaymentManagerError
as an argument, which is an error that is only constructed by storekit1, I believe. But commits proving me wrong are welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
afaa9ff
to
0ec8b55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
0ec8b55
to
6f21586
Compare
To aid with developing the server side changes for StoreKit2, I've added a button that generates a StoreKit2 receipt and sends it to the same endpoint on to our API. The button is only added in debug builds. Since this is a manager is let loose on the codebase they do not understand, please do review these changes diligently. XCode will complain about us not listening to transactions succeeding out-of-band - this is to be expected and we will fix this once we move to StoreKit2 properly - the changes I'm introducing here are not intended to be used in production.
This change is